-
Notifications
You must be signed in to change notification settings - Fork 795
[SYCL] Fix AddSecurityFlags having no side effects #17690
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[SYCL] Fix AddSecurityFlags having no side effects #17690
Conversation
CMake has a notion of scopes for variables and we fell into a pitfall of setting local variables instead of applying changes globally to the whole project. For reference, see: - https://cmake.org/cmake/help/v3.20/command/set.html - https://cmake.org/cmake/help/v3.20/command/include.html
@@ -31,7 +31,7 @@ macro(add_link_option_ext flag name) | |||
endif() | |||
endmacro() | |||
|
|||
function(append_common_extra_security_flags) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note for reviewers: updating set
calls on lines 14-16 to have PARENT_SCOPE
may seem like a solution, but there is a problem with append
(used on line 28) being a function instead of a macro. append
by itself contains PARENT_SCOPE
in set
it does, but it only propagates results one scope upwards (up to append_common_extra_security_flags
), whilst here we need two scopes two scopes up to reach the global scope.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
Testing on Arc was cancelled (by some unknown to me reason), but I do not expect this patch to have any functional impact and all other testing configurations are green, so I will proceed with merge |
CMake has a notion of scopes for variables and we fell into a pitfall of setting local variables instead of applying changes globally to the whole project. For reference, see: - https://cmake.org/cmake/help/v3.20/command/set.html - https://cmake.org/cmake/help/v3.20/command/include.html
This is a cherry-pick of #17690 --- CMake has a notion of scopes for variables and we fell into a pitfall of setting local variables instead of applying changes globally to the whole project. For reference, see: - https://cmake.org/cmake/help/v3.20/command/set.html - https://cmake.org/cmake/help/v3.20/command/include.html
CMake has a notion of scopes for variables and we fell into a pitfall of setting local variables instead of applying changes globally to the whole project. For reference, see: - https://cmake.org/cmake/help/v3.20/command/set.html - https://cmake.org/cmake/help/v3.20/command/include.html
This PR contains several cherry-picks and some unique changes which have not been applied to the `sycl` branch yet. The intent of this PR is to enable as much (quickly) possible hardening flags to be in better compliance with our SDL requirements. The main thing this PR is after are things like immediate bindings, fortify source, stack protection and `relro`. The thing that this PR is **not** after are extra warning flags - some of them we can't apply globally because LLVM itself isn't warning free, some of them we can't apply even locally to SYCL RT because we haven't fixed corresponding warnings yet. Patches which were cherry-picked from the `sycl` branch: - [SYCL] Fix AddSecurityFlags having no side effects (#17690) - Patch-By: Alexey Sachkov <[email protected]> - [SYCL] Refresh hardening flags applied to the project (#18398) - Patch-By: Nikita Kornev <[email protected]> - [SYCL][CMAKE] Refactor -fPIE handling (#19235) - Patch-By: Alexey Sachkov <[email protected]> - [SYCL][CMAKE] Drop nodlopen from hardening flags (#19357) - Patch-By: Alexey Sachkov <[email protected]> - [SYCL][CMAKE] Fix _FORTIFY_SOURCE=3 (#19268) - Patch-By: Alexey Sachkov <[email protected]> - [SYCL][CMake] Properly enable -pie hardening flag (#19447) - Patch-By: Alexey Sachkov <[email protected]> Additional changes which have **not** been applied to the `sycl` branch: - Adjusted `configure.py` to the new way of `-fPIE` handling - Dropped `/sdl` flag because LLVM isn't warning-free - it will be applied locally to SYCL RT in a separate PR against the `sycl` branch for future releases - Dropped `/analyze` flag because SYCL RT isn't warning-free - it will be applied locally to SYCL RT in a separate PR against the `sycl` branch for future releases
CMake has a notion of scopes for variables and we fell into a pitfall of setting local variables instead of applying changes globally to the whole project.
For reference, see: